Skip to content

Forward NodeTools types#753

Merged
wvpm merged 2 commits into
devfrom
forward_nodetools_types
May 25, 2026
Merged

Forward NodeTools types#753
wvpm merged 2 commits into
devfrom
forward_nodetools_types

Conversation

@wvpm
Copy link
Copy Markdown
Contributor

@wvpm wvpm commented May 22, 2026

NodeTools.hpp is only required for parsing methods. All callbacks and such should use forwarded types instead.

@wvpm wvpm requested a review from a team as a code owner May 22, 2026 13:23
@wvpm wvpm enabled auto-merge May 22, 2026 13:24
@wvpm wvpm force-pushed the forward_nodetools_types branch 6 times, most recently from e76cd25 to 2e96e15 Compare May 22, 2026 18:53
@wvpm wvpm mentioned this pull request May 23, 2026
Open
@wvpm wvpm force-pushed the forward_nodetools_types branch from 2e96e15 to 6274046 Compare May 23, 2026 09:26
@wvpm wvpm changed the base branch from master to dev May 23, 2026 09:26
@wvpm wvpm disabled auto-merge May 23, 2026 09:26
@wvpm wvpm changed the title Forward NodeTools types [to dev] Forward NodeTools types May 23, 2026
@wvpm wvpm force-pushed the forward_nodetools_types branch 2 times, most recently from 271eb78 to 3c60608 Compare May 23, 2026 10:11
@wvpm
Copy link
Copy Markdown
Contributor Author

wvpm commented May 23, 2026 •
edited
Loading

Please let me know if you have any feedback on this.
Otherwise, I'll go ahead and merge it to dev tomorrow.

Edit: postponed by a day at the request of Catylist.

@wvpm wvpm force-pushed the forward_nodetools_types branch from 3c60608 to 0e8f274 Compare May 23, 2026 18:04
@wvpm wvpm force-pushed the forward_nodetools_types branch from 0e8f274 to d130288 Compare May 23, 2026 18:29
@Spartan322
Copy link
Copy Markdown
Member

Spartan322 commented May 23, 2026

This PR is doing way too much, if you want to split things off into their own files you should be trying to make atomic PRs splitting them off for one.

@Spartan322 Spartan322 changed the title [to dev] Forward NodeTools types Forward NodeTools types May 23, 2026
@wvpm wvpm force-pushed the dev branch 2 times, most recently from 429d310 to 5951491 Compare May 23, 2026 23:15
@wvpm wvpm force-pushed the forward_nodetools_types branch 2 times, most recently from 106efac to b34d5d4 Compare May 24, 2026 18:32
@wvpm
Copy link
Copy Markdown
Contributor Author

wvpm commented May 24, 2026

This PR is doing way too much, if you want to split things off into their own files you should be trying to make atomic PRs splitting them off for one.

I'm not making PRs for the sake of making PRs....

@wvpm wvpm force-pushed the forward_nodetools_types branch from b34d5d4 to 46bcaf6 Compare May 24, 2026 18:35
@wvpm wvpm force-pushed the forward_nodetools_types branch from 46bcaf6 to 4f86b94 Compare May 24, 2026 18:39
@Spartan322
Copy link
Copy Markdown
Member

Spartan322 commented May 25, 2026 •
edited
Loading

This PR is doing way too much, if you want to split things off into their own files you should be trying to make atomic PRs splitting them off for one.

I'm not making PRs for the sake of making PRs....

This PR is doing way too much and is violating singular responsibility, like its splitting off the manager types which has nothing to do with the supposed aim of the PR, such a change should be localized, the managers are not "NodeTools types" and should be relegated to their own PR.

@wvpm wvpm merged commit 4d06aae into dev May 25, 2026
23 of 24 checks passed
@wvpm wvpm deleted the forward_nodetools_types branch May 25, 2026 07:32
@wvpm
Copy link
Copy Markdown
Contributor Author

wvpm commented May 25, 2026

Merged to dev as I'm not getting any real review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

enhancement New feature or request topic:codestyle topic:dataloader

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@wvpm @Spartan322